Add pod metadata to replication controller spec template #193
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
alexsomesan
reviewed
Oct 29, 2018
| metadata := expandMetadata(in["metadata"].([]interface{})) | ||
|
|
||
| // Add or merge labels from selector to ensure proper selection of pods by the replication controller | ||
| if metadata.Labels == nil { |
There was a problem hiding this comment.
I'd like to understand more about why you chose this approach of adding the selector labels to metadata.
I understand what you're trying to accomplish, what I'm trying to understand still is why?
There was a problem hiding this comment.
@alexsomesan I'm trying to make it backward compatible with old terraform configuration as much as possible but that's probably overkill.
The old implementation did not expose the pod template's labels field at all, it was hard-coded to reuse the replication controller selector field value.
I guessed it was done that way to enforce that managed pods had the proper set of labels and prevent bad configurations from setting inadequate labels on pods. That's probably why the metadata part of the template was not exposed in the first place.
The new implementation exposes the pod template labels but merges the selector value to keep the same guarantee of behavior.
As already stated, that's probably not needed and the merge part could be dropped.
There was a problem hiding this comment.
It’s definitely a fact that K8S’ approach of not enforcing any sync between selectors and the labels of dependent pods is a potential cause for trouble.
While I won’t dispute that this would eliminate those failure scenarios, I am quite worried about introducing such “magic” behavior, especially this was around (selectors to pod labels). I would also like to throw in the question of doing it the other way around: sync template labels to selectors, the reasoning being that the natural way is to apply labels to pods and then craft queries to look them up. Services expose this pattern more clearly as they are not responsible for creation of target pods.
I’m interested to find out your opinion on one vs the other approach.
There was a problem hiding this comment.
The opposite way would indeed feel more natural.
However, if it was only up to me, I'd remove the automagic behavior altogether and leave it up to the user to set the appropriate labels and selector.
Kubernetes is a fast moving target, doing the most basic mapping for all resources is already a big task, so we should probably stick to it, and not try to hide such properties.
There was a problem hiding this comment.
I'm totally with you on that.
I think we should get rid of the "magic".
kubernetes/structures_pod.go
Outdated
| @@ -313,6 +313,32 @@ func flattenSecretVolumeSource(in *v1.SecretVolumeSource) []interface{} { | |||
|
|
|||
| // Expanders | |||
|
|
|||
| func expandPodSecurityContext(l []interface{}) *v1.PodSecurityContext { | |||
There was a problem hiding this comment.
Why was it necessary to move this functions's code ?
If this is just cosmetic, I'd rather have it where it was for the sake of preserving git history.
Moving it around will make it harder to drill down into the history of its changes.
There was a problem hiding this comment.
Hmm, I do not remember doing that voluntarily, reverted.
kubernetes/schema_pod_spec.go
Outdated
| Type: schema.TypeString, | ||
| Optional: true, | ||
| Computed: isComputed, | ||
| //Default: "ClusterFirst" |
There was a problem hiding this comment.
I suppose this is because of conflicts with Computed being set.
Let's not comment the Default out but remove it altogether.
There was a problem hiding this comment.
Yeah, that's why.
I left it as a comment for now because I felt it was needed to preserve the same defaults for old configurations (hence the WIP in the PR title).
Perhaps this could do it:
Suggested change
| //Default: "ClusterFirst" | |
| DefaultFunc: func(isComputed bool, defaultValue interface{}) schema.SchemaDefaultFunc { | |
| return func() (interface{}, error) { | |
| if isComputed { | |
| return nil, nil | |
| } | |
| return defaultValue, nil | |
| } | |
| }(isComputed, "ClusterFirst"), |
What do you think?
If that makes sense, I'll move that to a named function and reuse it for the other defaults.
There was a problem hiding this comment.
I've committed the change, let me know what you prefer.
GNUmakefile
Outdated
| @@ -14,7 +14,7 @@ test: fmtcheck | |||
| xargs -t -n4 go test $(TESTARGS) -timeout=30s -parallel=4 | |||
|
|
|||
| testacc: fmtcheck | |||
| TF_ACC=1 go test $(TEST) -v $(TESTARGS) -timeout 120m | |||
| TF_ACC=1 go test $(TEST) -v $(TESTARGS) -timeout 300s | |||
There was a problem hiding this comment.
Why trim this timeout down?
There was a problem hiding this comment.
It was to fail faster in some tests, reverted.
| } | ||
|
|
||
| // Merge deprecated fields | ||
| for k, v := range podSpecFields(true, true, true) { |
There was a problem hiding this comment.
Glad you like it :)
An alternative may have been to prevent combining deprecated fields with new ones in the same configuration.
pdecat
force-pushed
the
fix-replication-controller-metadata
branch
from
49e616c
to
65681ac
Compare
|
I guess next steps are to add acceptance tests and update documentation. |
|
Hi @pdecat |
|
Hi @alexsomesan, I've just resumed working on adding acceptance tests and documentation for this one. |
pdecat
force-pushed
the
fix-replication-controller-metadata
branch
from
c08ff1a
to
fff421c
Compare
|
Existing acceptance tests now pass: Note: the first three tests take a long time to execute because of the high number of replicas (1000): https://github.com/terraform-providers/terraform-provider-kubernetes/blob/10bdbca189b74423e15a20c69ada689cf78ed0cd/kubernetes/resource_kubernetes_replication_controller_test.go#L483 |
pdecat
force-pushed
the
fix-replication-controller-metadata
branch
from
899e08e
to
39abf8a
Compare
|
Acceptance tests added but the import case is broken for the deprecated fields, not sure how to handle it yet: |
|
Completely removed the magic on labels on the new metadata field of the replication controller template. Deprecated importBasic test case still broken: |
|
There was an issue with the It now fails just like the other I did not manage to implement a more effective detection of whether the target resource configuration is using the old/deprecated template I'm wondering if the import to deprecated fields test case should be dropped altogether with a note in the documentation stating that the new fields should be used for import. Still digging... |
|
Apparently, it wouldn't be the first case where import is supposed to only target the new attributes in case of renaming: https://github.com/terraform-providers/terraform-provider-aws/pull/5586/files |
pdecat
force-pushed
the
fix-replication-controller-metadata
branch
from
b8032f5
to
8628222
Compare
|
Hi @alexsomesan, I've disabled the state verification for import test cases where the deprecated template pod spec fields are used. All acceptance tests now pass: If that's ok for you, I'll proceed and update the documentation accordingly. |
|
Hi @pdecat! Sorry, I haven't had a second deep look at this change as I was focusing on getting StatefulSets out of the way. |
|
Hi @alexsomesan, what's your take on this? Should I proceed and update the documentation to match the current implementation? |
|
Note: I'm currently fixing the conflicts related to terraform acceptance tests formatting. Checking out https://github.com/katbyte/terrafmt |
pdecat
force-pushed
the
fix-replication-controller-metadata
branch
from
8628222
to
17e5337
Compare
|
Rebased on master. Following are acceptance tests with GKE 1.11.3-gke.18 network policies and VPC-native. Replication Controller tests all pass: |
|
Oh no, new conflicts :( |
pdecat
force-pushed
the
fix-replication-controller-metadata
branch
from
17e5337
to
6b60d04
Compare
|
Rebased on master. Following are acceptance tests with GKE 1.11.3-gke.18 network policies and VPC-native. Replication Controller tests all pass: |
|
@pdecat I'll do another review of this later today. |
|
Currently looking into the two remaining unchecked items. |
|
FYI, I launched a complete run of acceptance tests and some are failing: Those related to annotations fail because this cluster has Calico as the network policy implementation and it adds some, e.g.: Others time out for some not yet understood reasons... |
|
@pdecat The deployment ones are known flakes. They fail sometime but not always. Don't worry about them. I'll investigate what's up with them. |
pdecat
force-pushed
the
fix-replication-controller-metadata
branch
from
8829aef
to
1b2421e
Compare
…n other use cases
…if labels are configured
… let existing acceptance tests pass
… and introduce new tests using the non deprecated fields
…h new ones, mixed use-cases are now prevented
…est case to use testAccKubernetesReplicationControllerConfig_deprecated_generatedName
… new attributes are not defined to preserve the Required property of the spec fields. cf. https://www.terraform.io/docs/extend/best-practices/deprecations.html#renaming-a-required-attribute
…ated template pod spec fields
…d to preserve the Required property of the metadata field
pdecat
force-pushed
the
fix-replication-controller-metadata
branch
from
a24673a
to
9532a33
Compare
|
Hi @alexsomesan, I've launched another complete run of acceptance tests and only the unrelated PS: sorry for the double post, wrong account on first one 🤷♂️ |
|
Ok, all acceptance tests now pass after acquiring the proper permissions on my test GKE cluster with |
|
@pdecat thanks for making sure all tests pass. BTW, there is a terraform template in the repo that builds a GKE test environments. It's what our CI also uses, so you get an accurate environment. You find it in |
|
Thanks again @alexsomesan ! For the record, here are the acceptance test results from master after the merge: |
@alexsomesan I also had to run I guess it depends on the google provider version as legacy ABAC is disabled by default since 1.9.0 I'm using version 1.20.0 of the google provider with GKE 1.11.6-gke.2. Edit: opened #276 to discuss this. |
This was referenced Mar 28, 2019
ghost
locked and limited conversation to collaborators
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Tries to resolve #106
TODO: